Skip to content

Improve score by supporting extra_phrase for extra words in rules #4432

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 14 commits into
base: develop
Choose a base branch
from
Open

Improve score by supporting extra_phrase for extra words in rules #4432

wants to merge 14 commits into from

Conversation

alok1304
Copy link
Collaborator

@alok1304 alok1304 commented Jun 19, 2025 •
edited by AyanSinhaMahapatra
Loading

Follow up of:

Add new phrases like extra_phrase this is special for extra-words. This phrase is represented in the format [[n]], where n indicates the maximum number of extra-words allowed at that position in the rule.

If extra-words appear at the correct position and their count does not exceed the allowed limit n, then the score is increased to 100.

Reference #4420

Tasks

  • Reviewed contribution guidelines
  • PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • Commits are in uniquely-named feature branch and has no merge conflicts 📁
  • Updated documentation pages (if applicable)
  • Updated CHANGELOG.rst (if applicable)

Signed-off-by: Alok Kumar alokkumarjipura9973@gmail.com

@alok1304 alok1304 force-pushed the improve-score-extra-words branch 3 times, most recently from 6b7da16 to 8fb9f47 Compare June 19, 2025 17:09
@alok1304 alok1304 force-pushed the improve-score-extra-words branch 8 times, most recently from ac99199 to 9b50bff Compare June 23, 2025 14:30
AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Jun 23, 2025
View reviewed changes
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alok1304! Looking much better

See comments for your consideration. I've updated your PR description to mention that this is a follow up PR, since there is important context and reviews in the previous PR, we need to preserve this as required.

src/licensedcode/detection.py Outdated
"""
Return True if any of the matches in ``license_matches`` List of LicenseMatch
has extra words are in the correct place.
"""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to check both a bit explicitly:

  1. For all the matches which have extra words, they are in correct location
  2. For all the matches which does not have extra words, they are correct detections

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And add a test accordingly

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And add a test accordingly

where I should add a test and like how I implement for all license_matches

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also i added test for is_extra_words_position_valid see: https://github.com/aboutcode-org/scancode-toolkit/pull/4432/files#diff-d1520ccce311f8f4d4932ba68589bc23b098f8090a307b0b440edb4a846ae21cR1325

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added test

@alok1304 alok1304 force-pushed the improve-score-extra-words branch from b9c7c16 to a50b3db Compare June 24, 2025 06:27
@alok1304
Copy link
Collaborator Author

alok1304 commented Jun 24, 2025 •
edited
Loading

I addet test for 3-seq where there is no detection of copyrights statements , Ref: https://github.com/xyzzy-022/xyzzy/blob/5a16eb998470241b33ad3caa6a4946d0448a16b6/LEGAL.md?plain=1#L97
this file when we scan we got extra-words so I added extra-phrase marker in that corresponding matched rule. Such that we can improve the score.

@alok1304 alok1304 force-pushed the improve-score-extra-words branch from 8a25b51 to 43c6bdb Compare June 24, 2025 07:44
alok1304 added 9 commits July 1, 2025 17:01
@alok1304
add test for correct position of extra-words and enhance `detection…
685a16f 
…_log`

Add test for is correct position of `extra-words` according to `extra-phrases` that is present in rules.

if we find `extra-words` are in the right place then we set score to `100`.
And also show in `detection_log` why we increasing the score to keep track of this.

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
Add new phrase for 'extra-words` in rules
760eba3 
Add new phrases like `extra_phrase` this is special for extra-words.
This phrase is represented in the format [[n]], where n indicates the maximum number of extra-words allowed at that position in the rule.

If extra-words appear at the correct position and their count does not exceed the allowed limit `n`, then the score is increased to `100`.

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
fix test failure
b784806 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
remove extra_phrase marker from rules
88196da 
due to `extra_phrase` in rules, this shows that rules containing `extra-words`

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
fix minor issues
f80963c 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
add test for 3-seq
6ad990e 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
improve detection and handling of extra-words and add test for 3-seq
5f090e3 
add a new `extra-phrase` for a rule i.e bsd-new

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
fix test failure
bf3417d 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
remove extra-phrase marker from index_tokenizer_with_stopwords
61284f4 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304 alok1304 force-pushed the improve-score-extra-words branch from b38f718 to 61284f4 Compare July 1, 2025 11:31
alok1304 added 2 commits July 1, 2025 19:09
@alok1304
get only extra-phrase-spans
490a081 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
add tests for extra-phrase removal in `index_tokenizer_with_stopwo…
060fd63 
…rds`

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
add test for 3-seq and extra-words-spans
68ab63d 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
@alok1304
Copy link
Collaborator Author

alok1304 commented Jul 2, 2025 •
edited
Loading

Actually, the previous test case is failing because of extra-phrase is removed from rules while loading, after that when scan same rules then matcher is 3-seq not any one of these"1-hash", "1-spdx-id", "2-aho" this is due to extra-phrase marker that are present in rules. This test case is failing https://github.com/aboutcode-org/scancode-toolkit/blob/develop/tests/licensedcode/test_detection_validate.py#L101

   def check_rule_or_license_can_be_detected_exactly(licensish):
        """
        Check that a rule or license can be detected exactly, either by thyself (or with an exact match
        to any rule for deprecated rules).
        """
        idx = cache.get_index()
        deadline = time() + 20  # ms
        matches = idx.match(query_string=licensish.text, _skip_hash_match=True, deadline=deadline)
        # ensure we can self-detect exactly
        expected = [licensish.identifier]
        results = [m.rule.identifier for m in matches]
    
        if results != expected:
            expected.append(f'file://{licensish.rule_file()}')
            assert results == expected
        icm = is_correct_detection(matches)
        if not icm:
            expected.append(f'file://{licensish.rule_file()}')
>           assert results == expected

E           AssertionError: assert ['bsd-new_newlib3.RULE'] == ['bsd-new_newlib3.RULE',\n 'file:///home/vsts/work/1/s/src/licensedcode/data/rules/bsd-new_newlib3.RULE']
E             Right contains one more item: 'file:///home/vsts/work/1/s/src/licensedcode/data/rules/bsd-new_newlib3.RULE'
E             Full diff:
E               [
E                'bsd-new_newlib3.RULE',
E             -  'file:///home/vsts/work/1/s/src/licensedcode/data/rules/bsd-new_newlib3.RULE',
E               ]

because is_correct_detection gives False because it not include 3-seq in matcher see: https://github.com/aboutcode-org/scancode-toolkit/blob/develop/src/licensedcode/detection.py#L1090
That's why the test case is failing. So, I added 3-seq see: https://github.com/aboutcode-org/scancode-toolkit/pull/4432/files#diff-5f6ccc1d8f9540cae715bddaf337246f4c69c48ca596b2086e05b18c541aef4aR1098

Now, all the test cases are passed :)

@alok1304
improve and add more test for is_extra_words_position_valid
5b933c0 
Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
alok1304
alok1304 commented Jul 2, 2025
View reviewed changes
src/licensedcode/detection.py
@@ -1087,7 +1095,7 @@ def is_correct_detection(license_matches):
]

return (
all(matcher in ("1-hash", "1-spdx-id", "2-aho") for matcher in matchers)
all(matcher in ("1-hash", "1-spdx-id", "2-aho", "3-seq") for matcher in matchers)
Copy link
Collaborator Author

@alok1304 alok1304 Jul 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i add 3-seq because of extra-phrase is removed from rules while loading, after that when we scan same rules, now matcher is 3-seq not any one of these "1-hash", "1-spdx-id", "2-aho" this is due to extra-phrase marker that are present in rules. That's why this gives 3-seq. We already loaded the rules where extra-phrase is removed, but not in the actual rule file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alok1304 we cannot add this here, is_correct_detection cannot have "3-seq" built in, you need to create a different function with different name as this is not the same anymore. Where are you using this or why ahave you added this, because we need to ensure rule texts are matched exactly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to create a new function for use at #4432 (comment), we cannot put 3-seq in the old is_correct_detection function because that disturbs the detection post-processing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to create a new function for use at #4432 (comment), we cannot put 3-seq in the old is_correct_detection function because that disturbs the detection post-processing.

okay, got it

@alok1304
add more tests for is_extra_words_at_valid_positions and improve de…
16d3364 
…tection

Signed-off-by: Alok Kumar <alokkumarjipura9973@gmail.com>
AyanSinhaMahapatra
AyanSinhaMahapatra requested changes Jul 7, 2025
View reviewed changes
Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alok1304. looking great overall, see comments for your consideration.

...dcode/data/plugin_license/extra-words/scan-extra-words-3-seq-license/with-copyrights/LICENSE
DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER
IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
Software License Agreement (BSD License)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NO need for this renaming, please keep this 3-seq test as-is. Same for the test expectation file and test name.

tests/licensedcode/data/plugin_license/extra-words/scan-extra-words-3-seq-license.expected.json
@@ -1,70 +0,0 @@
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to delete and add a new file with some other name, please restore this.

tests/licensedcode/data/plugin_license/extra-words/scan-extra-words-2-aho-license.expected.json
@@ -6,7 +6,7 @@
"license_expression_spdx": "BSD-3-Clause",
"detection_count": 1,
"detection_log": [
"extra-words"
"extra-words-permitted-in-rule"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test expectations look good, thanks!

src/licensedcode/detection.py
@@ -1087,7 +1095,7 @@ def is_correct_detection(license_matches):
]

return (
all(matcher in ("1-hash", "1-spdx-id", "2-aho") for matcher in matchers)
all(matcher in ("1-hash", "1-spdx-id", "2-aho", "3-seq") for matcher in matchers)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alok1304 we cannot add this here, is_correct_detection cannot have "3-seq" built in, you need to create a different function with different name as this is not the same anymore. Where are you using this or why ahave you added this, because we need to ensure rule texts are matched exactly?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AyanSinhaMahapatra AyanSinhaMahapatra AyanSinhaMahapatra requested changes

Assignees
No one assigned
Labels
None yet
Projects
GSoC 2025: Have variable license sections in license rules @alok1304
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@alok1304 @AyanSinhaMahapatra